fix: show pending questions reliably and open the right chat from notifications - #235
Conversation
…ifications Questions asked while the user was in another chat, or long before they opened the session, never rendered: - Recovery fetched `GET /question?directory=<selected workspace>`, but the selected workspace is rarely the session's own directory, so the query found nothing. Resolve the directory from the session being opened and retry transient failures. - Claude Code had no recovery path at all: `pendingQuestions` returned an empty list and the one-shot bridge event was the only delivery. Read the pending files back (only for sessions whose process is alive) and answer them, and make the decline button work via the bridge's deny response. - The Claude hook expired questions after ~75 s: the loop counted iterations of a quarter-second sleep as seconds. Measure real time, keep permissions at five minutes, and let questions wait up to the hook timeout (raised to an hour). - Dismissed cards came back on every refetch; remember the dismissal for the current visit to the session. Notification taps opened the wrong chat or nothing: - The deep link was a bare session id that was never consumed, so a second tap for the same session changed nothing. Deliver a one-shot tokenised link and consume the intent extras. - Question/complete/error notifications carried no runtime id, so a chat on another agent opened against the wrong backend. Include the runtime id in every notification and switch to it before opening (falling back to the session catalog for notifications posted before this change). - On a cold start with no runtime selected yet, `openSession` no-opped and the request was dropped; keep it pending until a runtime exists. - Answered questions now cancel their notification, and the wider notification-id/requestCode space stops unrelated sessions colliding.
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| onSessionIdle = { sessionId, title, runtimeId -> | ||
| notifications.notifySessionComplete(sessionId, title, runtimeId) | ||
| }, |
There was a problem hiding this comment.
[maintainability · low]
このラムダはパラメータをそのまま渡すだけのラッパーで、もともとのメソッド参照のままでもシグネチャが完全一致しているためそのまま書けます（onSessionIdle = notifications::notifySessionComplete 等）。ラムダ化しても機能差はなく冗長なので、可読性・簡潔性の観点からメソッド参照を維持する方が望ましいです。
Suggestion:
| onSessionIdle = { sessionId, title, runtimeId -> | |
| notifications.notifySessionComplete(sessionId, title, runtimeId) | |
| }, | |
| onSessionIdle = notifications::notifySessionComplete, |
| intent.removeExtra(RuntimeNotificationHelper.EXTRA_TARGET_SESSION_ID) | ||
| intent.removeExtra(RuntimeNotificationHelper.EXTRA_RUNTIME_ID) |
There was a problem hiding this comment.
[bug · low]
removeExtra による「extras の消費」は、同一プロセス内の設定変更（recreation）時に同じ Intent インスタンスが再配送されるケースには有効ですが、プロセス死亡後の復元では、システム（ActivityRecord）が保持する元インテントのコピーが再配送されるため removeExtra の変更が反映されず、最後の deep link が再発火して「すでに離れたチャットへ引き戻される」問題が残ります。コメントの担保範囲を「設定変更のみ」に限定するか、onSaveInstanceState に消費済みセッションIDを保存して onCreate で再発火を抑止するガードを追加することを検討してください。
| fun pendingQuestions(): List<QuestionRequest> = | ||
| permissionBridge | ||
| .pendingRequests() | ||
| .filter { it.kind == ClaudePermissionBridge.Kind.QUESTION } |
There was a problem hiding this comment.
[maintainability · low]
この filter は冗長です。toQuestionRequest は request.kind != Kind.QUESTION の場合に null を返すため（ClaudePermissionBridge.kt の実装）、後続の mapNotNull が既に非QUESTIONを除外します。フィルタを外して1パスにまとめるか、意図を明示するコメントを残すと読みやすくなります。
| .pendingRequests() | ||
| .filter { it.kind == ClaudePermissionBridge.Kind.QUESTION } | ||
| .filter { sessions[it.androidSessionId]?.process?.isAlive == true } | ||
| .mapNotNull(permissionBridge::toQuestionRequest) |
There was a problem hiding this comment.
[bug · medium]
mapNotNull(permissionBridge::toQuestionRequest) はパースできない質問ファイル（questions 配列が欠落・壊れている、旧フォーマット等）を静かに破棄します。イベント経路の pollPending() ではこのケースを PermissionAsked にフォールバックして拒否UIを提供していますが、リカバリ経路にはフォールバックがありません。その結果、リカバリ対象であるはずのブロックされたターンが、ホックのタイムアウト（約1時間）までカードも出ずに停滞し、ユーザーは回答も拒否もできません。pollPending() と同様にフォールバック（例: PermissionAsked として返す）を用意するか、無効なファイルを削除/期限切れ扱いにすることを検討してください。
| * that, denying the turn while the card was still on its way. The hook script's own timeout | ||
| * stays just under this so it gets to deny gracefully first. | ||
| */ | ||
| const val HOOK_TIMEOUT_SEC = 3600 |
There was a problem hiding this comment.
[bug · medium]
Claude Code のフック設定の timeout にはドキュメント上の上限（既定 60 秒、最大 600 秒）が設けられているため、3600 を設定しても実効的にはクランプされたり、無効とみなされる可能性があります。その場合、1時間待機という意図は実現せず、スクリプト側の質問タイムアウト（3540 秒）が有効な上限より大きく勝ってしまうため、元の「ターンが早期に拒否される」問題が解消されません。対象の Claude Code バージョンで timeout: 3600 が実際に有効に動作することを確認し、上限に合わせた値に調整することを推奨します。
1 participant
Summary
Fixes two reported bugs:
Root causes and fixes
Question rendering
GET /question?directory=<selected workspace>, but the composer's selected workspace is rarely the session's own directory, so the query found nothingbackend.session(id).directory), keep it inChatUiState.sessionDirectoryas the answer/reject fallback, and retry transient fetch failurespendingQuestionsreturnedemptyList()); the one-shot bridge event was the only deliveryClaudePermissionBridge.pendingRequests()reads pending files without consuming the watcher's one-shot emission;ClaudeCodeRuntime.pendingQuestions()exposes only questions whose session process is alive (a file left by a dead process can never be answered);ClaudeCodeTargetoverridespendingQuestionsClaudeCodeTarget.rejectQuestionwrites the bridge's deny response, failing the waiting tool call cleanlyTIMEOUT_SEC=300iterations)onQuestionResolvedcallbackNotification tap → session
LaunchedEffectnever re-ranChatDeepLink; intent extras are consumed so recreation can't re-navigateruntime_id; the deep link switches the runtime first (falling back to the session catalog for notifications posted before this change) and resolves the real chat titleopenSessionsilently no-opped and the pending request was dropped anywayrequestCode = sessionId.hashCode(), 12-bit id mask) let unrelated sessions overwrite each other's tap targetsVerification
pendingRequests, runtime id propagation (ChatViewModelQuestionTest,ClaudePermissionBridgeTest,RuntimeActivityRepositoryTest)../gradlew :app:testDebugUnitTest— 759 tests, 0 failures../gradlew detekt spotlessCheck :app:lintDebug :app:assembleDebug— green.Pre-PR review
判定: APPROVE
ブロッカー
提案（非ブロッキング）
チェック済み項目
git diff --check